-
Notifications
You must be signed in to change notification settings - Fork 128
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
read-cache.c: reduce unnecessary cache entry name copying #1249
Conversation
In function create_from_disk, we have already copy the cache entries name from disk or previous cache entry, we can reduce unnecessary copy before that. Signed-off-by: ZheNing Hu <adlternative@gmail.com>
/submit |
Submitted as pull.1249.git.1654436248249.gitgitgadget@gmail.com To fetch this version into
To fetch this version to local tag
|
On the Git mailing list, René Scharfe wrote (reply to this): Am 05.06.22 um 15:37 schrieb ZheNing Hu via GitGitGadget:
> From: ZheNing Hu <adlternative@gmail.com>
>
> In function create_from_disk, we have already copy the cache
> entries name from disk or previous cache entry, we can reduce
> unnecessary copy before that.
>
> Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> ---
> read-cache.c: reduce unnecessary cache entry name copying
>
> Index cache entries name are copied twice wrongly, so reduce the first
> one to fix it.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1249%2Fadlternative%2Fzh%2Frm-ce-copy-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1249/adlternative/zh/rm-ce-copy-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1249
>
> read-cache.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/read-cache.c b/read-cache.c
> index 96ce489c7c5..e61af3a3d4d 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1944,8 +1944,6 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,
> ce->ce_namelen = len;
> ce->index = 0;
> oidread(&ce->oid, ondisk->data);
> - memcpy(ce->name, name, len);
> - ce->name[len] = '\0';
This removal looks correct to me. The extra copying was added by
575fa8a3ed (read-cache: read data in a hash-independent way, 2019-02-19)
but its commit message doesn't mention it.
>
> if (expand_name_field) {
> if (copy_len)
Some more context lines would help to see the redundancy; here they are:
if (expand_name_field) {
if (copy_len)
memcpy(ce->name, previous_ce->name, copy_len);
memcpy(ce->name + copy_len, name, len + 1 - copy_len);
*ent_size = (name - ((char *)ondisk)) + len + 1 - copy_len;
} else {
memcpy(ce->name, name, len + 1);
*ent_size = ondisk_ce_size(ce);
}
>
> base-commit: ab336e8f1c8009c8b1aab8deb592148e69217085 |
User |
On the Git mailing list, Junio C Hamano wrote (reply to this): René Scharfe <l.s.r@web.de> writes:
>> diff --git a/read-cache.c b/read-cache.c
>> index 96ce489c7c5..e61af3a3d4d 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -1944,8 +1944,6 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,
>> ce->ce_namelen = len;
>> ce->index = 0;
>> oidread(&ce->oid, ondisk->data);
>> - memcpy(ce->name, name, len);
>> - ce->name[len] = '\0';
>
> This removal looks correct to me. The extra copying was added by
> 575fa8a3ed (read-cache: read data in a hash-independent way, 2019-02-19)
> but its commit message doesn't mention it.
I agree with the analysis. When we are prefix-compressing the
entry, name[] may be much shorter than len, so this memcpy() may
potentially be reading beyond the end of the on-disk buffer, so the
original code is not just redundant, but it may simply be wrong.
Thanks, both. Will queue.
>>
>> if (expand_name_field) {
>> if (copy_len)
>
> Some more context lines would help to see the redundancy; here they are:
>
> if (expand_name_field) {
> if (copy_len)
> memcpy(ce->name, previous_ce->name, copy_len);
> memcpy(ce->name + copy_len, name, len + 1 - copy_len);
> *ent_size = (name - ((char *)ondisk)) + len + 1 - copy_len;
> } else {
> memcpy(ce->name, name, len + 1);
> *ent_size = ondisk_ce_size(ce);
> }
>
>>
>> base-commit: ab336e8f1c8009c8b1aab8deb592148e69217085 |
This branch is now known as |
This patch series was integrated into seen via git@2d0146e. |
This patch series was integrated into seen via git@ef2aa39. |
This patch series was integrated into seen via git@9f8b8b0. |
There was a status update in the "New Topics" section about the branch Remove redundant copying (with index v3 and older) or possible over-reading beyond end of mmapped memory (with index v4) has been corrected. Will merge to 'next'. source: <pull.1249.git.1654436248249.gitgitgadget@gmail.com> |
This patch series was integrated into seen via git@37a506d. |
This patch series was integrated into next via git@760f43d. |
This patch series was integrated into seen via git@3140c98. |
This patch series was integrated into seen via git@4956152. |
There was a status update in the "Cooking" section about the branch Remove redundant copying (with index v3 and older) or possible over-reading beyond end of mmapped memory (with index v4) has been corrected. Will merge to 'master'. source: <pull.1249.git.1654436248249.gitgitgadget@gmail.com> |
This patch series was integrated into seen via git@d007935. |
This patch series was integrated into seen via git@113656e. |
This patch series was integrated into master via git@113656e. |
This patch series was integrated into next via git@113656e. |
Closed via 113656e. |
Index cache entries name are copied twice wrongly, so reduce the first one
to fix it.
cc: Junio C Hamano gitster@pobox.com
cc: brian m. carlson sandals@crustytoothpaste.net
cc: Christian Couder christian.couder@gmail.com
cc: René Scharfe l.s.r@web.de